feat: add structured data and API interaction tools - #869

🔍 PR Audit: structured-data-api-tools

Change: structured-data-api-tools
Branch: feat/structured-data-api-tools → main
Files: 29 files, 3,091 insertions, 14 deletions
New Tools: 6 (api, graphql, json, yaml, data, webhook)

Spec Compliance

Deviation: GraphQL uses native fetch instead of graphql-request (which was added as a dependency but never imported).

Task Completion

45/45 tasks from tasks.md are marked [x] and present in the diff. All implementation tasks were completed.

Test Coverage

Total: 53 tests, all passing.

Security

• ✅ URL filtering: scheme blocking (file://, gopher://, dict://), internal IP blocking, allowlist enforcement
• ✅ Auth: bearer, basic, API key — all properly formatted
• ✅ Response sanitization: Set-Cookie and WWW-Authenticate stripped
• ✅ GraphQL: depth limiting (default 10), complexity limiting (default 1000)
• ✅ Webhook: HMAC-SHA256 verification

⚠️ Concerns:

1. Dead dependency — graphql-request added to package.json but never imported (native fetch used instead)
2. Webhook createWebhook() accepts empty URL — no URL validation on the raw function
3. HMAC uses non-constant-time comparison — === instead of crypto.timingSafeEqual()
4. GraphQL depth/complexity are heuristic-based — can be circumvented via quoted strings/comments

Code Quality

• ✅ JSDoc on all public functions with @param and @returns
• ✅ Zod schemas with .describe() for LangChain tool descriptions
• ✅ Consistent { ok, data?, error? } return pattern
• ✅ *Impl function pattern for testability (plain object input)
• ✅ createXxxTool() factory pattern for LangChain tool creation

⚠️ Minor:

1. serializeYaml error message says "Invalid JSON input" (should be "Invalid input")
2. saveWebhooks directory creation uses fire-and-forget .then() with silent catch

Gaps (Post-Merge Follow-ups)

1. Remove dead graphql-request dependency from package.json
2. Add integration tests for GraphQL tool
3. Add allowlist matching tests in filterUrl
4. Add timeout and response size limit tests
5. Add introspectSchema test
6. Add URL validation to createWebhook() or document it as intentional

Verdict: APPROVED

Comprehensive implementation following project patterns. All spec requirements met. Test coverage is solid for unit tests; integration tests exist for network tools. Minor concerns are low-risk and can be addressed post-merge.

Audit performed against OpenSpec change structured-data-api-tools (proposal.md, design.md, tasks.md, 6 spec files).

PR #869 Audit Report

What was implemented correctly

REST API Client (api.js):

• All 6 OpenSpec spec requirements met: authenticated requests (Bearer, Basic, API Key), URL allowlist enforcement, configurable timeouts, response header sanitization, response body size limiting (10MB default)
• Uses native fetch (no extra dependency) - matches design decision feat: create llm-tui-harness spec and design artifacts #2
• Permission tier network:outbound is correct

GraphQL Client (graphql.js):

• Query/mutation execution with variables, operationName, timeouts - all spec scenarios covered
• URL allowlist enforcement present
• Depth/complexity limits present (heuristic-based, documented as known limitation in design doc)
• Permission tier network:outbound is correct

JSON Manipulation (json.js):

• Parse, serialize, transform, filter (JSONPath), access - all 5 actions implemented
• Uses jsonpath-plus as specified in design decision docs: update AGENTS.md for Node.js without uv/TypeScript #4
• Permission tier filesystem:read is correct

YAML Manipulation (yaml.js):

• Parse, serialize, transform, filter, access - all 5 actions implemented
• Uses js-yaml as specified in design decision feat: add LLM TUI harness change (proposal, design, specs, tasks) #5
• Permission tier filesystem:read is correct

Data Transformation (data.js):

• JSON to YAML, JSON to CSV, YAML to CSV conversions - all 6 actions implemented
• Mapping rules for CSV conversions
• Uses csv-parse/csv-stringify as specified in design decision feat: implement llm-tui-harness core architecture #6
• Permission tier filesystem:read is correct

Webhook Management (webhook.js):

• Create, list, delete, verify (HMAC-SHA256) - all 4 actions implemented
• Persistent storage to memory/tools/webhooks.json
• Permission tier filesystem:read,filesystem:write is correct

Security (all fixed):

• URL allowlist enforcement on all network tools (api, graphql, webhook)
• Sensitive headers stripped from REST responses
• Response body size limiting on REST API tool
• HMAC constant-time comparison (fixed)
• URL validation on webhook create (fixed)
• Dead graphql-request dependency removed (fixed)

Code quality:

• Follows existing tool pattern (Zod schema -> impl function -> registration)
• JSDoc comments on all public functions
• 1,346 tests pass (all existing + new)
• Lint/format clean

Missing from the spec/issue

1. GraphQL schema introspection (OpenSpec spec requirement + issue #788)

• The OpenSpec spec explicitly states: "The GraphQL client SHALL support schema introspection queries" with a scenario for executing __schema introspection.
• The issue feat: add structured data and API interaction tools #788 also lists "schema introspection" in the proposed solution.
• Not implemented. The GraphQL tool only supports query and mutation - no introspection action.

2. Rate limiting (issue #788 security considerations)

• Issue states: "Implement client-side rate limiting (default: 10 requests/second) to avoid triggering provider blocks."
• Not implemented on any network tool (api, graphql, webhook).
• This is a nice-to-have, not a blocker.

Extra features not explicitly requested

None significant. The maxBodySize parameter on the REST API tool is actually covered by the issue's security considerations ("Limit response body size (default: 10MB)"), so it's not extra - it's expected.

Bugs & Issues

1. Dead format parameter on JSON and YAML tools

• Both json.js and yaml.js have a format field in their Zod schemas that is accepted but never used in the implementation.
• The data tool correctly uses format for input validation, but the individual JSON/YAML tools don't need it since they only handle their own format.
• Fix: Remove the format field from the JSON and YAML tool schemas.

2. GraphQL depth/complexity are heuristic-based (acknowledged in design doc)

• The implementation counts {/} characters for depth and uses regex for complexity. This is documented as a known risk in the design doc.
• Acceptable as a first pass - a full graphql-js parser would be a much heavier dependency.
• The limits are configurable, so users can tune them.

Security Assessment

Good:

• URL allowlist enforcement on all network tools (api, graphql, webhook)
• Disallowed schemes blocked (file://, gopher://, dict://)
• Internal IP rejection via filterUrl
• Sensitive headers stripped from REST responses
• Response body size limiting (10MB default)
• Configurable timeouts on all HTTP operations
• HMAC constant-time comparison
• URL validation on webhook create
• No hardcoded secrets, no eval(), no sync fs

Acceptable:

• Heuristic-based GraphQL depth/complexity (documented limitation, configurable)
• No rate limiting (mentioned in issue but not critical for first pass)

No critical security issues found.

Does it make sense for the harness & LLM?

Yes. The tools:

• Follow the existing pattern (Zod schema -> impl function -> registration)
• Have appropriate permission tiers
• Are well-documented with JSDoc
• Have comprehensive tests
• Are classified for relevant agent types (search, research, coding, documentation, debug)
• Provide structured, validated interfaces that the LLM can reason about (no shell commands, no ad-hoc curl/jq)

Summary

Recommendation: Merge with two small follow-ups:

1. Add GraphQL introspection support (action: introspect on the graphql tool)
2. Remove unused format parameter from JSON and YAML tool schemas

The PR is solid, well-structured, and addresses the core need: structured API interaction tools for the harness.